fix(app): make multi-file environment import resilient and idempotent#8374
fix(app): make multi-file environment import resilient and idempotent#8374NicolasCV wants to merge 4 commits into
Conversation
Importing multiple environment files at once aborted the whole batch on a single bad or mismatched file, left partial imports behind a generic error, and silently piled up "X copy" duplicates on re-import. - Parse each file independently with its own format detection, so one malformed file or a mixed Bruno/Postman selection no longer aborts the import. - Import each environment in its own try/catch and collect failures instead of throwing mid-loop after some were already written. - Skip environments whose sanitized name already exists (collection or global scope), deduping within the batch too, so re-import is a no-op. - Replace the generic toast with one summary: imported count plus an itemized skipped/unnamed/failed note; keep the modal open only on total failure.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe environment import modal now reads each selected file independently, detects file formats per input, skips duplicate names, and reports imported, skipped, unnamed, and failed environments. ChangesEnvironment import flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js (1)
31-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove the import workflow into a co-located hook.
The component now owns file parsing, duplicate checks, Redux writes, and toast side effects. Extracting this into a
useImportEnvironmenthook would keep the modal focused on UI wiring. As per coding guidelines, “MUST: Prefer custom hooks for business logic, data fetching, and side-effects in React.”Also applies to: 109-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js` around lines 31 - 92, The import workflow logic in ImportEnvironmentModal is too mixed into the component, including parsing, duplicate detection, Redux dispatches, and toast handling. Move the processEnvironments flow and related side effects into a co-located custom hook such as useImportEnvironment, and keep ImportEnvironmentModal focused on UI/state wiring by calling the hook for the import action and result handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js`:
- Line 11: The batch import flow in ImportEnvironmentModal still aborts when
readMultipleFiles(Array.from(files)) hits one bad file, so change the
file-processing path to handle each selected file independently. Update the
logic around readMultipleFiles and the per-file importer in
ImportEnvironmentModal so a malformed JSON only affects that file, while valid
files continue through the existing import routine.
- Around line 36-61: Normalize environment names once in ImportEnvironmentModal
before both deduping and importing, instead of sanitizing only for the Set key.
In the ImportEnvironmentModal loop, use the sanitized value from sanitizeName
consistently for the seen check, the toImport list, and the
addGlobalEnvironment/importEnvironment payloads so the global and collection
branches behave the same and never re-import raw names.
- Around line 31-92: The environment import flow in processEnvironments needs
test coverage for the new batch summary states. Add/extend tests under
tests/environments/import-environment to cover mixed Bruno/Postman selections,
malformed files with valid imports, duplicate-name skips, unnamed environments,
and partial dispatch failures, asserting the toast summary and returned
imported/skipped/unnamed/failures counts from ImportEnvironmentModal.
---
Nitpick comments:
In
`@packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js`:
- Around line 31-92: The import workflow logic in ImportEnvironmentModal is too
mixed into the component, including parsing, duplicate detection, Redux
dispatches, and toast handling. Move the processEnvironments flow and related
side effects into a co-located custom hook such as useImportEnvironment, and
keep ImportEnvironmentModal focused on UI/state wiring by calling the hook for
the import action and result handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8a00a39-8925-4a36-b5b7-a06e0856732e
📒 Files selected for processing (1)
packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js
| const processEnvironments = async (environments, parseFailures = []) => { | ||
| const failures = [...parseFailures]; | ||
|
|
||
| const named = []; | ||
| let unnamed = 0; | ||
| for (const env of environments) { | ||
| if (env.name && env.name !== 'undefined') named.push(env); | ||
| else unnamed++; | ||
| } | ||
|
|
||
| const existing = isGlobal ? globalEnvironments : collection?.environments || []; | ||
| const seen = new Set(existing.map((e) => sanitizeName(e.name || ''))); | ||
|
|
||
| const toImport = []; | ||
| let skipped = 0; | ||
| for (const env of named) { | ||
| const key = sanitizeName(env.name); | ||
| if (seen.has(key)) { | ||
| skipped++; | ||
| } else { | ||
| toast.error('Failed to import environment: env has no name'); | ||
| return false; | ||
| seen.add(key); | ||
| toImport.push(env); | ||
| } | ||
| }); | ||
|
|
||
| if (validEnvironments.length === 0) { | ||
| toast.error('No valid environments found to import'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Process environments sequentially to ensure unique name checking considers previously imported environments | ||
| let importedCount = 0; | ||
| for (const environment of validEnvironments) { | ||
| let imported = 0; | ||
| for (const environment of toImport) { | ||
| try { | ||
| const action = isGlobal | ||
| ? addGlobalEnvironment({ name: environment.name, variables: environment.variables, color: environment.color }) | ||
| : importEnvironment({ name: environment.name, variables: environment.variables, color: environment.color, collectionUid: collection?.uid }); | ||
|
|
||
| await dispatch(action); | ||
| importedCount++; | ||
| imported++; | ||
| } catch (error) { | ||
| console.error(`Failed to import environment "${environment.name}":`, error); | ||
| failures.push({ name: environment.name, message: error?.message || String(error) }); | ||
| } | ||
| } | ||
|
|
||
| if (imported > 0) { | ||
| toast.success(`Imported ${imported} environment${imported > 1 ? 's' : ''}.`); | ||
| } | ||
|
|
||
| toast.success(`${importedCount > 1 ? `${importedCount} environments` : 'Environment'} imported successfully`); | ||
| } catch (error) { | ||
| toast.error('An error occurred while importing the environment(s)'); | ||
| console.error(error); | ||
| throw error; | ||
| const notes = []; | ||
| if (skipped > 0) notes.push(`${skipped} already existed and ${skipped > 1 ? 'were' : 'was'} skipped`); | ||
| if (unnamed > 0) notes.push(`${unnamed} had no name`); | ||
| if (failures.length > 0) { | ||
| const names = failures.map((f) => f.name).slice(0, 3).join(', '); | ||
| const more = failures.length > 3 ? ` and ${failures.length - 3} more` : ''; | ||
| notes.push(`${failures.length} failed (${names}${more})`); | ||
| } | ||
|
|
||
| if (notes.length > 0) { | ||
| const message = notes.join('; ') + '.'; | ||
| if (failures.length > 0) toast.error(message); | ||
| else toast(message); | ||
| } else if (imported === 0) { | ||
| toast.error('No valid environments found to import.'); | ||
| } | ||
|
|
||
| return { imported, skipped, unnamed, failures }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for tests covering the Import Environment modal and new batch outcome messages.
# Expected: specs or tests asserting mixed-format import, duplicate skips, malformed-file tolerance, and summary feedback.
rg -n --glob '*.{js,jsx,ts,tsx}' \
'ImportEnvironmentModal|import-environment|import-global-environment|already existed|No valid environments|failed \(|had no name|mixed.*Postman|mixed.*Bruno' \
packages/bruno-app testsRepository: usebruno/bruno
Length of output: 7650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the modal implementation.
sed -n '1,240p' packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js
printf '\n--- TEST FILES ---\n'
# List relevant import-environment tests.
find tests/environments/import-environment -type f \( -name '*.spec.ts' -o -name '*.spec.js' -o -name '*.spec.jsx' -o -name '*.spec.tsx' \) | sort
printf '\n--- COLLECTION BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/collection-env-import/collection-env-import.spec.ts
printf '\n--- GLOBAL BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/global-env-import/global-env-import.spec.ts
printf '\n--- COLLECTION POSTMAN/ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/collection-env-import.spec.ts
printf '\n--- GLOBAL ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/global-env-import.spec.ts
printf '\n--- COLOR IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/env-color-import/env-color-import.spec.tsRepository: usebruno/bruno
Length of output: 34003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the modal implementation.
sed -n '1,240p' packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js
printf '\n--- TEST FILES ---\n'
# List relevant import-environment tests.
find tests/environments/import-environment -type f \( -name '*.spec.ts' -o -name '*.spec.js' -o -name '*.spec.jsx' -o -name '*.spec.tsx' \) | sort
printf '\n--- COLLECTION BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/collection-env-import/collection-env-import.spec.ts
printf '\n--- GLOBAL BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/global-env-import/global-env-import.spec.ts
printf '\n--- COLLECTION POSTMAN/ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/collection-env-import.spec.ts
printf '\n--- GLOBAL ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/global-env-import.spec.ts
printf '\n--- COLOR IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/env-color-import/env-color-import.spec.tsRepository: usebruno/bruno
Length of output: 34003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the modal implementation.
sed -n '1,240p' packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js
printf '\n--- TEST FILES ---\n'
# List relevant import-environment tests.
find tests/environments/import-environment -type f \( -name '*.spec.ts' -o -name '*.spec.js' -o -name '*.spec.jsx' -o -name '*.spec.tsx' \) | sort
printf '\n--- COLLECTION BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/collection-env-import/collection-env-import.spec.ts
printf '\n--- GLOBAL BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/global-env-import/global-env-import.spec.ts
printf '\n--- COLLECTION POSTMAN/ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/collection-env-import.spec.ts
printf '\n--- GLOBAL ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/global-env-import.spec.ts
printf '\n--- COLOR IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/env-color-import/env-color-import.spec.tsRepository: usebruno/bruno
Length of output: 34003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js
printf '\n--- TEST FILES ---\n'
find tests/environments/import-environment -type f \( -name '*.spec.ts' -o -name '*.spec.js' -o -name '*.spec.jsx' -o -name '*.spec.tsx' \) | sort
printf '\n--- COLLECTION BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/collection-env-import/collection-env-import.spec.ts
printf '\n--- GLOBAL BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/global-env-import/global-env-import.spec.ts
printf '\n--- COLLECTION POSTMAN/ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/collection-env-import.spec.ts
printf '\n--- GLOBAL ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/global-env-import.spec.ts
printf '\n--- COLOR IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/env-color-import/env-color-import.spec.tsRepository: usebruno/bruno
Length of output: 34003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js
printf '\n--- TEST FILES ---\n'
find tests/environments/import-environment -type f \( -name '*.spec.ts' -o -name '*.spec.js' -o -name '*.spec.jsx' -o -name '*.spec.tsx' \) | sort
printf '\n--- COLLECTION BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/collection-env-import/collection-env-import.spec.ts
printf '\n--- GLOBAL BRUNO IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/bruno-env-import/global-env-import/global-env-import.spec.ts
printf '\n--- COLLECTION POSTMAN/ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/collection-env-import.spec.ts
printf '\n--- GLOBAL ENV IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/global-env-import.spec.ts
printf '\n--- COLOR IMPORT ---\n'
sed -n '1,220p' tests/environments/import-environment/env-color-import/env-color-import.spec.tsRepository: usebruno/bruno
Length of output: 34003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant test files and search for the new batch outcome assertions.
rg -n -A 4 -B 4 \
'already existed|had no name|No valid environments found to import|failed \(|toast\.success|toast\.error|ImportEnvironmentModal|import-environment' \
tests/environments/import-environment packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.jsRepository: usebruno/bruno
Length of output: 18189
Add batch outcome coverage for environment imports Add tests for the new summary states in tests/environments/import-environment/*: mixed Bruno/Postman selections, one malformed file alongside valid files, duplicate skips, unnamed environments, and partial import failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js`
around lines 31 - 92, The environment import flow in processEnvironments needs
test coverage for the new batch summary states. Add/extend tests under
tests/environments/import-environment to cover mixed Bruno/Postman selections,
malformed files with valid imports, duplicate-name skips, unnamed environments,
and partial dispatch failures, asserting the toast summary and returned
imported/skipped/unnamed/failures counts from ImportEnvironmentModal.
Source: Coding guidelines
Read each selected file on its own so a single malformed JSON no longer aborts the whole batch — valid files continue through the import routine and the bad file is reported in the summary. Normalize environment names with sanitizeName once before deduping and importing so the global and collection branches behave the same and never re-import raw names. Add tests covering mixed Bruno/Postman selections, malformed-with-valid imports, duplicate-name skips, unnamed environments, partial dispatch failures, and name normalization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.spec.js (1)
120-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a same-batch duplicate case.
This suite checks duplicates against existing environments, but the new
seen.add(name)path also deduplicates duplicates within the imported batch. A test for two incoming environments collapsing to the same sanitized name would cover the… copyregression this PR is meant to prevent. As per coding guidelines, "Add tests for any new functionality or meaningful changes."Also applies to: 177-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.spec.js` around lines 120 - 133, The current import-environment tests cover duplicates against existing environments, but not duplicates within the same imported batch handled by the new seen.add(name) path. Add a test in the ImportEnvironmentModal spec, near the existing duplicate cases in renderModal/dropFiles, that imports two environments whose sanitized names collapse to the same value (including the “copy” suffix regression) and assert the second is skipped while the first is created, with the expected toast and no extra dispatches.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.spec.js`:
- Around line 78-83: The unnamed Bruno test is bypassing the real importer
behavior, so it does not cover the actual contract for missing names. Update the
mock in ImportEnvironmentModal/index.spec.js to mirror importBrunoEnvironment by
normalizing unnamed environments to "Imported Environment" before returning from
passthrough, so processEnvironments() is exercised the same way as the real
Bruno importer. Keep the assertion focused on the Bruno import flow and the
unnamed case that feeds the importer contract.
---
Nitpick comments:
In
`@packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.spec.js`:
- Around line 120-133: The current import-environment tests cover duplicates
against existing environments, but not duplicates within the same imported batch
handled by the new seen.add(name) path. Add a test in the ImportEnvironmentModal
spec, near the existing duplicate cases in renderModal/dropFiles, that imports
two environments whose sanitized names collapse to the same value (including the
“copy” suffix regression) and assert the second is skipped while the first is
created, with the expected toast and no extra dispatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8accbd45-c78a-42a6-811b-56114ae0db5c
📒 Files selected for processing (2)
packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.jspackages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/Environments/Common/ImportEnvironmentModal/index.js
Guard non-string names before sanitizing and treat names that sanitize to empty as unnamed, so a malformed name can't throw or import a blank-named environment. Normalize the name once in the named/unnamed split and reuse it for the dedupe key and import payloads. Make the unnamed-environment test use the Postman path (the Bruno importer defaults missing names) and add coverage for non-string / empty-after-sanitize names.
|
Went through CodeRabbit's review. Here's where each point landed:
Attachments:
bruno-fix.environments.mp4
|
|
Pushed 79ce252 for the two follow-up review comments. The within-batch duplicate path now has its own test. Two environments in the same drop whose names sanitize to the same value ('Prod/Env' and 'Prod-Env' both become 'Prod-Env'): the first one imports, the second is skipped, and neither gets a 'copy' suffix. It asserts the toast summary and that dispatch only fires once, so the I also made the Bruno importer mock faithful to the real one. It now normalizes a missing name to 'Imported Environment' before returning, the same as |
Fixes #8373
Description
Importing several environment files at once is fragile. One bad file makes the whole batch fail, a failure partway through leaves some environments written to disk behind a generic error, and re-importing the same files quietly creates
… copyduplicates. This PR reads and parses each file on its own so the batch survives bad input, skips environments that already exist, and replaces the generic toast with a short summary of what happened.Root cause
Three separate problems in
handleImportEnvironmentandprocessEnvironments:Every file was read up front in a single
readMultipleFiles(Array.from(files))call and the format was detected from the first file only (detectEnvironmentFormat(parsedFiles[0].content)), then one importer was run over every file. A malformed file makes the read throw and aborts the whole import; a selection that mixes Bruno and Postman exports throws inside the importer and aborts the rest. Either way nothing gets imported.processEnvironmentslooped withawait dispatch(action)inside a singletry. If the third of five environments failed, the first two were already written to disk, the loop threw, and the user sawAn error occurred while importing the environment(s)with no idea which ones made it.The client never checks existing names. The only guard is the Electron handler
renderer:create-environment, which callsgenerateUniqueName(), so re-importing the same file producesX copy, thenX copy 2, and so on.The fix
Each file is now read and parsed on its own — the loop calls
readMultipleFiles([file])per file and detects its format individually, so a single malformed JSON only fails that file and is collected as a failure instead of aborting the batch. A mixed Bruno/Postman selection imports each file with its own detected format.Each environment is dispatched in its own
try/catchand failures are collected, so one failure doesn't take down the rest of the batch.Before importing, the code builds a set of existing names (
collection.environmentsfor collection scope, theglobalEnvironmentsselector for global scope) keyed bysanitizeName, the same function the Electron handler uses, and skips any name that already exists. The incoming batch is also deduped against itself. The name is normalized once withsanitizeNameand that value is reused for the dedupe check and for theimportEnvironment/addGlobalEnvironmentpayloads, so the collection and global branches behave identically and never re-import raw names. Re-importing the same file is now a no-op instead of stacking copies.Feedback is one summary: an
Imported N environments.toast, plus a note of how many were skipped, unnamed, or failed. The modal stays open only when nothing was imported and nothing was skipped so the user can retry; otherwise it closes and firesonEnvironmentCreatedonly if something was actually imported.The change is contained to the modal component. Importers, actions, and the Electron handler are untouched, and it reuses the existing
readMultipleFiles,importPostmanEnvironment,importBrunoEnvironment,sanitizeName,importEnvironment, andaddGlobalEnvironment.Before / After
X copy,X copy 2N environments imported successfullyor a generic errorImported N environments.plus a skipped/unnamed/failed noteVerification
Tested manually in the app, in both collection and global scope:
copyduplicates on disk.ESLint passes on the changed files. The change is contained to the modal component, with no API or behavior changes to importers, actions, or the Electron handler. Added a Jest spec for the modal (
ImportEnvironmentModal/index.spec.js, 8 tests, all passing) covering mixed Bruno/Postman selections, a malformed file alongside valid ones, duplicate-name skips, unnamed environments, partial dispatch failures, the "nothing valid found" path, and name normalization across the collection and global branches.Summary by CodeRabbit
New Features
Bug Fixes
Tests